controllers/hostedcontrolplane/hostedcontrolplane.go (1)

328-332: SkipInfrastructureHealthCheck also skips internal monitoring object deployment — consider naming clarity.

This flag now controls three distinct behaviors: skipping HCP readiness checks, skipping VPC endpoint checks, and skipping internal monitoring deployment (Routes + RouteMonitors). The name "skip-infrastructure-health-check" implies only the health-check portion. While this is consistent with the test-environment use case (lightweight HCPs), the overloaded semantics could surprise operators who set the flag expecting only health checks to be bypassed. A rename (e.g., skip-infrastructure-checks) or documentation note would reduce ambiguity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@controllers/hostedcontrolplane/hostedcontrolplane.go` around lines 328 - 332,
The config flag SkipInfrastructureHealthCheck is currently used in
deployInternalMonitoringObjects, readiness checks, and VPC endpoint checks which
overloads its meaning; update RHOBSConfig and all callers (e.g.,
deployInternalMonitoringObjects, any readiness check functions, and VPC endpoint
check code) to use a clearer name such as SkipInfrastructureChecks (or add an
explicit SkipMonitoringDeployment boolean) and propagate the rename/added field
everywhere it is read, or alternatively add a comment/docstring near RHOBSConfig
and the deployInternalMonitoringObjects function clarifying that
SkipInfrastructureHealthCheck also skips internal monitoring deployment; ensure
tests and any config parsing accept the new field or alias the old name to avoid
breaking behavior.

go.mod (1)

3-3: Consider bumping to go 1.25.7 (or go 1.26).

Go 1.25.7 (released 2026-02-04) includes security fixes to the go command and crypto/tls package. Go 1.26 was released on February 10, 2026 with further security improvements. The go directive in go.mod sets the minimum required version, so pinning to 1.25.5 still allows building with newer toolchains, but updating the directive ensures CI and downstream consumers aren't misled about the minimum tested version. At minimum, bumping to go 1.25.7 would pick up the crypto/tls security fix.

-go 1.25.5
+go 1.25.7

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@go.mod` at line 3, Update the Go module version in go.mod by changing the go
directive from "go 1.25.5" to at least "go 1.25.7" (or "go 1.26") so CI and
consumers reflect the minimum tested toolchain; edit the go directive line in
go.mod to the chosen version and commit the change.

controllers/hostedcontrolplane/hostedcontrolplane_test.go (1)

845-996: TestHostedControlPlaneReconciler_getRHOBSConfig has no coverage for the new SkipInfrastructureHealthCheck field.

None of the existing test cases sets "skip-infrastructure-health-check" in the ConfigMap data, and none of the expected structs asserts SkipInfrastructureHealthCheck: true. Since the field was introduced in this PR and getRHOBSConfig should read it from the ConfigMap (matching how only-public-clusters is handled), at least two cases are missing:

1. ConfigMap with "skip-infrastructure-health-check": "true" → expect SkipInfrastructureHealthCheck: true
2. ConfigMap with "skip-infrastructure-health-check": "false" (or absent) → expect SkipInfrastructureHealthCheck: false

♻️ Example test case to add

{
    name: "ConfigMap present with skip-infrastructure-health-check true - propagates field",
    configMap: &corev1.ConfigMap{
        ObjectMeta: metav1.ObjectMeta{
            Name:      configMapName,
            Namespace: config.OperatorNamespace,
        },
        Data: map[string]string{
            "probe-api-url":                      "https://api.example.com/probes",
            "probe-tenant":                       "some-tenant",
            "oidc-client-id":                     "some-id",
            "oidc-client-secret":                 "some-secret",
            "oidc-issuer-url":                    "https://issuer.example.com",
            "only-public-clusters":               "false",
            "skip-infrastructure-health-check":   "true",
        },
    },
    fallbackConfig: fallbackConfig,
    expected: RHOBSConfig{
        ProbeAPIURL:                   "https://api.example.com/probes",
        Tenant:                        "some-tenant",
        OIDCClientID:                  "some-id",
        OIDCClientSecret:              "some-secret",
        OIDCIssuerURL:                 "https://issuer.example.com",
        OnlyPublicClusters:            false,
        SkipInfrastructureHealthCheck: true,
    },
},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@controllers/hostedcontrolplane/hostedcontrolplane_test.go` around lines 845 -
996, TestHostedControlPlaneReconciler_getRHOBSConfig is missing coverage for the
new SkipInfrastructureHealthCheck field; update the test table in
TestHostedControlPlaneReconciler_getRHOBSConfig to include at least two cases
that exercise the ConfigMap key "skip-infrastructure-health-check": one where
the ConfigMap Data sets it to "true" and the expected RHOBSConfig has
SkipInfrastructureHealthCheck: true, and one where it is "false" or absent and
expected is false; ensure the ConfigMap entries and expected RHOBSConfig objects
in those test cases match how getRHOBSConfig parses "only-public-clusters" (use
the same parsing/trim logic) so the new field is asserted correctly.

test/e2e/route_monitor_operator_tests.go (3)

1111-1131: Use k8serrors.IsNotFound instead of string matching for not-found detection.

Same concern as the conflict check — string-based error matching is fragile. k8serrors.IsNotFound(err) is the idiomatic approach.

♻️ Suggested fix

 	err := k8s.Get(ctx, crdName, "", u)
 	if err != nil {
-		if strings.Contains(err.Error(), "not found") || strings.Contains(err.Error(), "NotFound") {
+		if k8serrors.IsNotFound(err) {
 			return false, nil
 		}
 		return false, err
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/route_monitor_operator_tests.go` around lines 1111 - 1131, In
crdExists, replace fragile string matching on err.Error() with the idiomatic
k8serrors.IsNotFound check: after calling k8s.Get(ctx, crdName, "", u), if err
!= nil use k8serrors.IsNotFound(err) to return (false, nil) for not-found and
otherwise return (false, err); keep the rest of the unstructured
GroupVersionKind logic intact and import k8s.io/apimachinery/pkg/api/errors as
k8serrors if not already imported.

---

870-926: Use k8serrors.IsConflict instead of string matching for conflict detection.

Line 913–914 detects optimistic concurrency conflicts by checking for substrings in the error message. This is fragile and locale-dependent. Use the typed error check instead.

♻️ Suggested fix

+	"k8s.io/apimachinery/pkg/api/errors" // already imported as k8serrors
 ...
 		// Check if it's a conflict error
-		if strings.Contains(err.Error(), "object has been modified") ||
-			strings.Contains(err.Error(), "the object has been modified") {
+		if k8serrors.IsConflict(err) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/route_monitor_operator_tests.go` around lines 870 - 926, The retry
loop in setHostedControlPlaneAvailable currently detects update conflicts by
checking substrings in the error text, which is fragile; replace that
string-matching logic with k8serrors.IsConflict(err) (importing
"k8s.io/apimachinery/pkg/api/errors" as k8serrors) when inspecting the error
returned by k8s.Update(ctx, u) so conflicts trigger the existing retry/backoff
path and non-conflict errors still return immediately; keep the same retry
count, log call (GinkgoLogr.V...), and sleep behavior but use
k8serrors.IsConflict to detect optimistic concurrency conflicts.

---

1245-1286: OIDC client secret stored in a ConfigMap rather than a Secret.

createRMOConfigMap writes oidc-client-secret into a ConfigMap. ConfigMaps are not encrypted at rest (by default) and have weaker RBAC expectations than Secrets. While this mirrors the existing operator config pattern (the operator reads from the same ConfigMap), storing credentials in a ConfigMap is a security concern worth documenting or migrating in the future.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/route_monitor_operator_tests.go` around lines 1245 - 1286, The
function createRMOConfigMap currently writes the sensitive oidc client secret
into a ConfigMap; instead create/update a Kubernetes Secret (Opaque) to hold
creds.ClientSecret (key "oidc-client-secret") and remove that key from the
ConfigMap data. Modify createRMOConfigMap to: 1) create/update a Secret object
for the OIDC secret (use creds.ClientSecret and the same namespace
"openshift-route-monitor-operator"), handling existing Secret via Get + Update
and using creds.ClientSecret as the value; 2) stop placing "oidc-client-secret"
into the ConfigMap.Data and only put non-sensitive fields (probe-api-url,
oidc-client-id, oidc-issuer-url, skip-infrastructure-health-check) into the
ConfigMap; and 3) ensure Update/Create logic mirrors the existing
Get/Update/Create pattern used in createRMOConfigMap for both the ConfigMap and
the new Secret. Ensure the secret key name matches "oidc-client-secret" so
consumers can locate it.

controllers/hostedcontrolplane/healthcheck.go (1)

66-70: Consider adding a log statement when skipping health checks.

When SkipInfrastructureHealthCheck is true, the function silently returns true. Adding a debug-level log here would help operators confirm the skip is taking effect and diagnose unexpected behavior in environments where this flag may be inadvertently set.

🔧 Suggested improvement

 func (r *HostedControlPlaneReconciler) hcpReady(ctx context.Context, hostedcontrolplane *hypershiftv1beta1.HostedControlPlane, cfg RHOBSConfig) (bool, error) {
 	// Skip health check for test environments (e.g., osde2e tests without real kube-apiserver)
 	if cfg.SkipInfrastructureHealthCheck {
+		ctrl.LoggerFrom(ctx).V(1).Info("Skipping HCP health check due to SkipInfrastructureHealthCheck flag")
 		return true, nil
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@controllers/hostedcontrolplane/healthcheck.go` around lines 66 - 70, Add a
debug log when hcpReady short-circuits due to
RHOBSConfig.SkipInfrastructureHealthCheck: inside
HostedControlPlaneReconciler.hcpReady, before returning true,nil when
cfg.SkipInfrastructureHealthCheck is set, call the reconciler's logger (or ctx
logger) to emit a debug message that clearly states the health check was skipped
and includes identifying context (e.g., hostedcontrolplane.Name/Namespace and
the fact SkipInfrastructureHealthCheck=true) so operators can confirm the skip
is taking effect.

main.go (1)

176-182: Same one-way boolean override issue as in getRHOBSConfig.

This mirrors the pattern flagged in hostedcontrolplane.go Line 136: the ConfigMap can only enable the flag, never disable it when the CLI default is true. If you apply the bidirectional fix in getRHOBSConfig, apply it here too for consistency.

♻️ Suggested fix

-		if configData.SkipInfrastructureHealthCheck {
-			setupLog.V(1).Info("Using skip-infrastructure-health-check from ConfigMap", "skipInfrastructureHealthCheck", configData.SkipInfrastructureHealthCheck)
-			skipInfrastructureHealthCheck = configData.SkipInfrastructureHealthCheck
-			configMapParams = append(configMapParams, "skip-infrastructure-health-check")
-		} else {
-			flagParams = append(flagParams, "skip-infrastructure-health-check")
-		}

Note: In getConfigFromConfigMap (Line 389), the raw "true" string check is fine since the field will be true only if the ConfigMap explicitly says "true". But in the consumer block above, checking configData.SkipInfrastructureHealthCheck (a bool) means the ConfigMap said "true" — you'd also need to check if the key existed but was set to "false" in order to override the CLI. Consider using a tri-state (string or *bool) in OperatorConfig to distinguish "not set" from "set to false".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@main.go` around lines 176 - 182, The current consumer logic using
configData.SkipInfrastructureHealthCheck only detects when the ConfigMap set the
value to true and cannot override a CLI default true to false; update the
handling so the OperatorConfig can represent tri-state (e.g., use *bool or
string for SkipInfrastructureHealthCheck) and in the consumer branch (where
skipInfrastructureHealthCheck, configMapParams and flagParams are set) check for
existence/explicit false as well as true; specifically change
OperatorConfig.SkipInfrastructureHealthCheck to a pointer or string in
getConfigFromConfigMap, preserve the existing check for the raw "true" there,
and in the block that assigns skipInfrastructureHealthCheck and appends to
configMapParams/flagParams, test nil vs non-nil (or explicit "false") so the
ConfigMap can both enable and disable the flag (override CLI) rather than only
enabling it.

controllers/hostedcontrolplane/hostedcontrolplane.go (1)

136-138: Boolean config values can only be enabled, never disabled, via ConfigMap override.

The pattern if configMap == "true" { cfg.X = true } means that once the CLI flag sets SkipInfrastructureHealthCheck = true, the ConfigMap cannot override it back to false. The same applies to OnlyPublicClusters (Line 133). If the intent is for the ConfigMap to be the authoritative source (as described in the PR summary: "ConfigMap overrides CLI"), consider parsing both "true" and "false" explicitly:

♻️ Suggested fix for bidirectional override

-	if strings.TrimSpace(configMap.Data["skip-infrastructure-health-check"]) == "true" {
-		cfg.SkipInfrastructureHealthCheck = true
-	}
+	if v := strings.TrimSpace(configMap.Data["skip-infrastructure-health-check"]); v != "" {
+		cfg.SkipInfrastructureHealthCheck = v == "true"
+	}

The same pattern should apply to only-public-clusters at Line 133–135 for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@controllers/hostedcontrolplane/hostedcontrolplane.go` around lines 136 - 138,
The ConfigMap override currently only sets cfg.SkipInfrastructureHealthCheck
(and cfg.OnlyPublicClusters) to true when the value is "true", preventing a
ConfigMap from disabling a flag set by CLI; change the logic to parse explicit
boolean values from the ConfigMap (e.g. use strconv.ParseBool on
configMap.Data["skip-infrastructure-health-check"] and
configMap.Data["only-public-clusters"]) and, if parsing succeeds, assign the
parsed bool to cfg.SkipInfrastructureHealthCheck and cfg.OnlyPublicClusters
respectively so the ConfigMap becomes the authoritative source for enabling or
disabling these flags.

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

test/e2e/README.md (1)

20-22: ⚠️ Potential issue | 🟡 Minor

Add language identifiers to diagram fences.

These three ASCII/diagram blocks still open with bare backticks, which triggers MD040 and can cause inconsistent rendering. Use ```text for the diagrams.

🛠️ Suggested fix

-```
+```text
 HostedControlPlane CR → RMO → RHOBS API → Synthetics Agent → Prometheus/Blackbox Exporter

-```
+```text
 ┌─────────────────────────────────────────────────────────────┐
 │ Test Process                                                 │
 ...
 └─────────────────────────────────────────────────────────────┘

-```
+```text
 ┌─────────────────────────────────────────────────────────────┐
 │ Real OpenShift Cluster (Integration/Staging)                │
 ...
 └─────────────────────────────────────────────────────────────┘

Also applies to: 288-314, 432-467

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/README.md` around lines 20 - 22, Update the three ASCII diagram code
fences so they include a language identifier to satisfy MD040; replace the
opening triple backticks for each diagram (for example the block that starts
with "HostedControlPlane CR → RMO → RHOBS API → Synthetics Agent →
Prometheus/Blackbox Exporter" and the two larger box diagrams starting
"┌─────────────────────────────────────────────────────────────┐") with ```text
so each diagram block begins with ```text and ends with ``` to ensure consistent
markdown rendering.

test/e2e/route_monitor_operator_tests.go (2)

855-858: Prefer API conflict detection helpers.

String-matching conflict messages is brittle; use k8serrors.IsConflict(err) for correctness across API versions.

🛠️ Suggested fix

-		if strings.Contains(err.Error(), "object has been modified") ||
-			strings.Contains(err.Error(), "the object has been modified") {
+		if k8serrors.IsConflict(err) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/route_monitor_operator_tests.go` around lines 855 - 858, Replace the
brittle string-based conflict detection that checks err.Error() with the
Kubernetes helper k8serrors.IsConflict(err): remove the strings.Contains(...)
checks and use k8serrors.IsConflict(err) in the conditional that decides to
retry; add the import alias (k8serrors) for "k8s.io/apimachinery/pkg/api/errors"
if not present so the code compiles. Ensure the retry path stays the same (the
block that currently runs when the string match is true) but is triggered by
k8serrors.IsConflict(err) instead.

---

1015-1024: Use k8s error helpers instead of string matching.

AlreadyExists, Forbidden, and NotFound should be detected via k8serrors helpers; message matching is fragile.

🛠️ Suggested fix

-		if err := installCRDFromYAML(ctx, k8s, crd.yamlDef); err != nil {
-			// Check if CRD already exists (race condition or detection failure)
-			if strings.Contains(err.Error(), "AlreadyExists") || strings.Contains(err.Error(), "already exists") {
+		if err := installCRDFromYAML(ctx, k8s, crd.yamlDef); err != nil {
+			// Check if CRD already exists (race condition or detection failure)
+			if k8serrors.IsAlreadyExists(err) {
 				GinkgoLogr.Info("CRD already exists (detected during create), skipping", "crd", crd.name)
 				continue
 			}
 			// Check if this is a permission error
-			if strings.Contains(err.Error(), "Forbidden") || strings.Contains(err.Error(), "forbidden") {
+			if k8serrors.IsForbidden(err) {
 				GinkgoLogr.Error(err, "Permission denied - cannot install CRD", "crd", crd.name)
 				missingCRDs = append(missingCRDs, crd.name)
 				continue
 			}

-	if err != nil {
-		// Check if it's a "not found" error
-		if strings.Contains(err.Error(), "not found") || strings.Contains(err.Error(), "NotFound") {
+	if err != nil {
+		// Check if it's a "not found" error
+		if k8serrors.IsNotFound(err) {

Also applies to: 1063-1067

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/e2e/route_monitor_operator_tests.go` around lines 1015 - 1024, The error
checks in the installCRDFromYAML error handling should use Kubernetes error
helpers instead of fragile string matching; update the conditional logic around
installCRDFromYAML (and the similar block handling errors later) to call
k8serrors.IsAlreadyExists(err), k8serrors.IsForbidden(err) and
k8serrors.IsNotFound(err) (importing k8s.io/apimachinery/pkg/api/errors as
k8serrors) and adjust the branches that log and append to missingCRDs
accordingly (keep existing messages and behavior but replace
strings.Contains(err.Error(), "...") with the k8serrors helper calls for
functions like installCRDFromYAML and wherever the duplicate check appears).

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting